getDecorator creates the impression that an existing decorator function is returned. Can we use createDecorator instead? I think that's the usual convention in Factory Design Pattern.

Can we change the type of target to Function & {definition?: ModelDefinition} so that this cast is not needed?

Why do we have to cast the result of PropertyDecoratorFactory.getDecorator to PropertyDecorator? Since we are calling PropertyDecoratorFactory, there should have been already enough type information for tsc to let it know that getDecorator is returning a PropertyDecorator.

I think this points out a flaw in our factory design.

nitpick:

allowInheritance: boolean = true

not sure will we have other options like allowInheritance in the future, how about making the third parameter an option object?

It sounds good to me.

Set a reference to target for a given spec if it's an object
Not sure what this was meant to say, TBH.

Please throw an error here. While this code may not be called from a TypeScript codebase, I believe it's still possible to call this decorator manually from JavaScript code base. Even if it wasn't, it still a good practice to throw an error if the program ever reaches a code/branch that we did not anticipate.

IIUC, describeInjectedArguments is reading metadata set by @inject, which uses ParameterDecoratorFactory.createDecorator internally.

I'd like this code to be decoupled from Reflector and use a helper method from @loopback/metadata to read the information stored by ParameterDecoratorFactory.createDecorator.

I think MetadataInspector is the tool to use here?

Similarly here - can we use a (possibly new) helper from @loopback/metadata to access property-level information?

Also the old code was recursing through the prototype chain while this new code is not, does it mean that Reflector.getMetadata is taking care of that already? I would expect a flag to control whether prototypes are consulted for metadata. If there is one (and is set to true by default), then please be more explicit here and set the flag to that default value, to make the intent more obvious to people reading this code for the first time.

AFAICT, bluebird is not used by this package - could you please remove?

AFAICT, bluebird is not used by this package - could you please remove?

I think we should not be using Symbols and use strings instead. Here is the use case where Symbols don't work:

1. There are two modules EXT and APP, each depending on a different version of @loopback/metadata. As a result, there are two copies of @loopback/metadata in node_modules tree. As a result, there are two TARGET symbols inside the Node.js process, one symbol is used by EXT and another symbol is used by APP.

2. APP module sets metadata using its TARGET symbol.

3. EXT module wants to read metadata set by APP. Because it has different TARGET symbol, it cannot read information set by APP.

Perhaps {[methodName: string]: Function} would be a more appropriate type than any? I think the type of "a n object with arbitrary methods" may be useful in other places too and therefore is worth sharing via an explicit type. (We are using it for Controllers for example.)

These two methods (processInherited and processLocal) look too similar to me, it's hard to tell what are the important differences and what is the shared boiler-plate. Could you please try another round of extracting shared code to clean this up?

For example, the following block of code can be easily extracted into a shared helper method.

// processInherited
    let methodMeta = baseMeta[method];
    if (methodMeta == null) {
      // Initialize the method metadata
      methodMeta = new Array(
        this.getNumberOfParameters(target, methodName),
      ).fill(undefined);
      baseMeta[method] = methodMeta;
    }

// processLocal
    let methodMeta = localMeta[method];
    if (methodMeta == null) {
      // Initialize the method metadata
      methodMeta = new Array(
        this.getNumberOfParameters(target, methodName),
      ).fill(undefined);
      localMeta[method] = methodMeta;
    }

With my proposed helper:

// processInherited
  const methodMeta = getOrCreateMeta(baseMeta, target, method);

// processLocal
  const methodMeta = getOrCreateMeta(localMeta, target, method);

// getOrCreateMeta(meta, target, method)
    let methodMeta = meta[method];   
    if (methodMeta == null) {
      // Initialize the method metadata
      methodMeta = new Array(
        this.getNumberOfParameters(target, method || ''),
      ).fill(undefined);
      meta[method] = methodMeta;
    }
    return methodMeta;

Can we use MetadataInspector in these tests please? That way the tests are decoupled from the internal implementation details of *DecoratorFactory.

Could you please beef up this README? I'd like to make this module useful to people outside LoopBack community too, so that we can gain more usage and more contributions. In order to get that, we need the README to be compelling enough for people who discover this package via search on npmjs.com.

See our README guidelines here: https://loopback.io/doc/en/contrib/README-guidelines.html. "Basic use" is the part I am missing most, e.g. how to use a decorator factory to create a decorator and then how to read the metadata stored by the decorator using the inspector.

I am ok with landing this pull request with the current insufficient README as long as the README is updated very soon afterwards, preferably before releasing this new package to npmjs.

It would be nice to include a short explanation of the common mistakes people make when using reflect-metadata and how this package is taking care of those tricky parts.

Sorry for not catching this earlier. Boolean arguments are an anti-pattern - how are the readers of the code invoking this method going to understand what true or false means?

const meta = getClassMetadata('mykey', myTarget, false);

Such API is also difficult to extend later, when we need to add more flags than just ownOnly.

I am proposing to change all methods accepting a boolean ownOnly argument to accept an "options" or "flags" object.

interface InspectOptions {
  ownOnly?: boolean
}

static getClassMetadata<T>(
  key: string,
  target: Function,
  options: InspectOptions = {})

I wish we did not have to use low-level Reflector API here and leverage either class-level decorator factory or something like MetadataInspector.setClassMetadata (a new method I just made up now).

Not a big deal, the patch can be landed with the current implementation too.

I think it would be better to use TSDoc comment here.

  /**
   * Inspect own metadata of a given target only,
   * ignore any metadata inherited from parent classes.
   * By default, inspector returns both own and inherited metadata.
   */
   ownMetadataOnly?: boolean;